-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
공지사항 목록 컴포넌트 / 공지사항 상세 컴포넌트 UI 구현 / 공지사항 ROUTER 설정 #749
Conversation
⚡️ Lighthouse report!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
상수화랑 시맨틱 태그 관련 코멘트 남겼습니다😄
/** | ||
* yyyy-mm-dd 형식 | ||
*/ | ||
createdAt: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StringDate 타입 재사용 어떠신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제안해주신대로 적용해보았습니다
theme="blank" | ||
onClick={() => { | ||
navigate('/notices'); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PATH.NOTICE 어떠신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 제안 감사합니다
@@ -0,0 +1,25 @@ | |||
import { styled } from 'styled-components'; | |||
|
|||
export const Container = styled.div` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
div 대신 main 이나 section 어떠신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제안해주셔서 감사해요 main으로 했습니다~
<> | ||
<NoticeDetailPage /> | ||
<RouteChangeTracker /> | ||
</> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 공지사항 목록이랑 상세도 RouteChangeTracker 를 추가해주셨군요👍👍
fe-리뷰완 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떻게 이렇게 빠르게 만드시는 거죠???
대단해요 우스
페이지가 여러 개이고 새로운 컴포넌트도 여러 개 인데 빠르게 만드는 능력 부럽습니다!
몇 가지 제안 및 코멘트 확인해주세요!!
고생하셨습니다!!!!
interface NoticeDetailProps { | ||
title: string; | ||
content: string; | ||
createdAt: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 그 StringDate로 하지 않으신 이유가 있으신가요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice 타입을 다른 PR에서 해서 타입 적용하는데 어려움이 있었어요 ㅠㅠ
제안해주신대로 StringDate로 변경했습니다
export const Title = styled.h1` | ||
margin-top: 20px; | ||
|
||
font: var(--text-title); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 조금 작은거 같은데 제가 알림 PR에다가 이거보다 큰 --text-page-title인가,, 만들었는데 나중에 그걸로 바꿔도 좋을 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋아요!
white-space: pre-wrap; | ||
`; | ||
|
||
export const ButtonContainer = styled.div` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
데스크탑에서는 하단에 홈으로 가기 버튼이 없어도 괜찮을 것 같아요!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/** | ||
* yyyy-mm-dd 형식 | ||
*/ | ||
createdAt: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DateString처럼 이것도 타입 강화하는건 어떻게 생각하시나요?
createdAt: string; | ||
} | ||
export default function NoticeItem({ id, title, createdAt }: NoticeItemProps) { | ||
const createdDate = createdAt.slice(0, 10); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
들어오는 내용이 yyyy-mm-dd 형식
인데 방어코드로 한번 더 잘라주는 건가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
시간은 안보여주는 디자인으로 구현해서 잘라서 보여줬어요
yyyy-mm-dd 형식으로욤
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헷갈리는 주석을 때문에 혼동이 와서 주석을 삭제했어요 🔥
|
||
font: var(--text-body); | ||
|
||
transition: color 0.2s ease-in-out; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
옹 hover를 위한 애니메이션인가요???
&:hover {
color: rgba(51, 122, 183, 1);
}
여기 안에 넣지 않으신 이유가 있나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hover안에 transition을 두면 마우스를 치웠을 때는 transition 속성이 작동하지 않아서 hover 밖에 위치시켜주었어요
/** | ||
* yyyy-mm-dd 형식 | ||
*/ | ||
createdAt: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
모든 공지 컴포넌트에서 yyyy-mm-dd 형식
으로 사용된다면 api에서 처음에 데이터를 변환해주시는건 어떻게 생각하세요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제안해주신대로 api에서 데이터를 변환시켜 주었습니다 👍👍👍
구현하는 과정에서 StringDateOnly 타입이 추가되었어요
/**
* yyyy-mm-dd HH-MM
*/
export type StringDate = `${number}-${number}-${number} ${number}:${number}`;
/**
* yyyy-mm-dd
*/
export type StringDateOnly = `${number}-${number}-${number}`;
<Layout isSidebarVisible isChannelTalkVisible={false}> | ||
<S.Container> | ||
<S.Title tabIndex={0}>보투게더 소식</S.Title> | ||
<NoticeList noticeList={MOCK_NOTICE_LIST} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
페이지에서 공지사항리스트를 주입시키신다면 서스펜스와 에러바운더리 처리가 어려울 것 같은데,
혹시 api요청은 페이지에서 하실 예정이신가요?? 아님 공지 목록에서 하실 예정이신가요??🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공지 목록을 감싸는 NoticeListFetcher를 만들려고 했었습니다
</> | ||
), | ||
}, | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{
path: PATH.NOTICES,
element: (
<>
<NoticeListPage />
<RouteChangeTracker />
</>
),
errorElement: <ErrorPage />,
children: [
{
path: ':noticeId',
element: (
<>
<NoticeDetailPage />
<RouteChangeTracker />
</>
),
},
],
};
이런 코드는 어떠세요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fe-리뷰완 |
두 분이 제안해주신 내용을 코드로 반영해보았습니다 구현 과정에서 StringDateOnly 타입이 새로 생겼어요 🔥 fe-리뷰요청 |
같은 파일에 있어야 코드 가독성이 높아질 것으로 생각했습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
피드백 반영된 코드 확인했습니다~ 타입명 관련 코멘트 하나 남겼어요!
fe-리뷰완
frontend/src/types/time.ts
Outdated
/** | ||
* yyyy-mm-dd | ||
*/ | ||
export type StringDateOnly = `${number}-${number}-${number}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
day까지만 포함시키겠다는 의미로
StringDateUpToDay 어떠신가요? (gpt의 도움을 받았습니다)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
더 명확하게 표현되는 것 같아 반영했습니다~
🔥 연관 이슈
close: #741
📝 작업 요약
⏰ 소요 시간
3시간
🔎 작업 상세 설명
-.VoTogether.-.Brave.2023-10-16.00-12-48.mp4
-.VoTogether.-.Brave.2023-10-16.00-14-01.mp4
-.VoTogether.-.Brave.2023-10-16.00-25-29.mp4
/notices : 공지사항 목록 페이지
/notices/:id 공지사항 상세 페이지